-
Notifications
You must be signed in to change notification settings - Fork 33
Fix type instability for exponent iterator with no variable #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
src/comparison.jl
Outdated
function Base.IteratorSize(it::ExponentsIterator{M,Nothing}) where {M} | ||
if isempty(it.object) | ||
return Base.HasLength() | ||
end | ||
return Base.IsInfinite() | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
IteratorSize
should be define on the type, not the value. -
This method, defined on the value, is not consistent with the above method, defined on the type.
-
Maybe just return
SizeUnknown()
? That's not precise, but it's accurate and type stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IteratorSize
should be define on the type, not the value.
It is but if you look at the implementation of collect(::Any)
, it is calling IteratorSize
on the value which then redirect to calling IteratorSize
on the type so it this implementation actually passes all my tests checking consistency with collect
etc...
So the only issue will be that users that call IteratorSize
on the type in the case they created the exponent iterator with a list of 0 variables will get Base.IsInfinite
while in fact the iterator only has one element. The fact that this gives IsInfinite
will just prevent some code to work while it could have worked if it was finite but that code is already not working when there is more than 0 variables so I don't see a use case being broken by that. I actually need IsInfinite
for the use with. Indeed, SizeUnknown
would be a bit safer here but on the other hand, the case with zero variable is kind of a corner case and IsInfinite
may also be understood as MaybeIsInfinite
, not sure if there is any code that relies on it being actually infinite :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the good compromise it to just return SizeUnknown
when it's called on the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah this is exactly what's done for Iterators.Cycle
, thanks for the link, this is exactly the same case, interesting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IteratorSize
should be define on the type, not the value.It is but if you look at the implementation [...]
-
I interpret the iteration interface like so:
-
Callers of
IteratorSize(::Any)
should be able to assume the call is "free", in the sense it gets constant folded and does not exist at run time when everything is concretely inferred. -
The intention behind the
IteratorSize(x) = IteratorSize(typeof(x))
is as a mere convenience for making it unnecessary to calltypeof
. So this method is supposed to be the only method ofIteratorSize
that accepts non-Type
. -
Callers should be able to assume
IteratorSize(x) === IteratorSize(typeof(x))
, if!isa(x, Type)
. -
If one wants a trait like
IteratorSize
, but defined on instances, they should create a new function instead of adding methods toIteratorSize
.
-
-
Another point possibly worth considering is abstract inference: when the argument types to a call are not concretely known, Julia will possibly consider a set of more than one matching method. Thus adding a methods to a callable can negatively affect the code generation (and resistance to invalidation) for unrelated loaded packages. This is especially bad if the method is of an uncommon type.
# in an instance or on the type. `Iterators.Cycle` has the same behavior, | ||
# see https://github.com/JuliaLang/julia/pull/54187 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Iterators.Cycle
has the same behavior
That's not correct, the PR is not merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, I wasn't planning to merge this one either until that one is merged or reaches a consensus.
The current approach causes small type instability which amplifies into bigger one down the line in JuliaAlgebra/MultivariateBases.jl#57 just for the sake of having
Base.IteratorSize
type-stable.Since
Base.IteratorSize
isn't so critical, it's probably better to have everything type-stable expectIteratorSize
.This is the approach taken in this PR.